-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Previewnet temporary fix for missing timestamps #245
Conversation
WalkthroughA temporary fix was implemented in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// todo remove after previewnet, temp fix to mock some of the timestamps | ||
if block.Timestamp == 0 { | ||
first := uint64(1715189257) | ||
blockTime := uint64(200) | ||
firstRecordedTimestampBlock := uint64(5493) | ||
|
||
diff := firstRecordedTimestampBlock - block.Height | ||
timestamp := first - blockTime*diff | ||
|
||
blockResponse.Timestamp = hexutil.Uint64(timestamp) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the temporary fix is clearly marked and documented.
The temporary fix for missing timestamps is clearly marked with a TODO comment. However, it would be beneficial to add more context about why this fix is necessary and any potential impacts. This will help future maintainers understand the reason behind this code and when it can be safely removed.
Consider adding a more detailed comment like this:
// TODO: Remove after previewnet, temp fix to mock some of the timestamps
// This temporary fix addresses the issue of missing timestamps in previewnet.
// It calculates a mock timestamp based on a predefined starting point and block time.
// This fix should be removed once the underlying issue is resolved.
blockTime := uint64(200) | ||
firstRecordedTimestampBlock := uint64(5493) | ||
|
||
diff := firstRecordedTimestampBlock - block.Height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just check diff doesn't get negative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did in a local test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add an if statement tho, but then what value to assign :D
firstRecordedTimestampBlock := uint64(5493) | ||
|
||
diff := firstRecordedTimestampBlock - block.Height | ||
timestamp := first - blockTime*diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be + ? I also think you might multiply it by second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be - because what I did is checked what is the first timestamp we have indexed, which is: 1715189257
then I subtract 200
(average block timestamp change I calculated) as many times as there is a diff from the height of the block that has this value and the block we are generating. Basically height 0 would have biggest change and height 5492
which is one less than first recorded timestamp would have lowest change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have better documented, but this will all go away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left some suggestions.
This PR is a TEMPORARY FIX on previewnet for missing timestamps.
Summary by CodeRabbit